Clean up expression implementations - #589

Review

WARNING this Claude-generated, reviewed by me; some accompanying fixes are in #590

Reviewed the full diff (18 files) against master, plus surrounding context in expr.hpp, expr_ptr.hpp, product.hpp, sum.hpp, op.hpp, wick.impl.hpp and tensor_network/v1.cpp. Line references are as of ab3531c.

The risky mechanical rewrites all check out:

• prefactor.as<Product>() *= *factor (wick.impl.hpp:834) — prefactor is ex<CProduct>(...), and CProduct does not override type_id(), so is<Product>() holds and Product::operator*= is reached exactly as before.
• canon_byproduct.as<Constant>() *= *bp (tensor_network/v1.cpp:461) — canon_byproduct is initialized ex<Constant>(1) at line 62 and never rebound.
• The as<Sum>() +=/-= and as<Product>() *= rewrites in expr_ptr.cpp are all inside is<Sum>()/is<Product>() guards, and neither CProduct nor NCProduct ever overrode those operators, so the devirtualization is behavior-preserving.
• operator==(const ExprPtr&, const ExprPtr&) keeps its hidden-friend declaration at expr_ptr.hpp:87, so moving the definition into expr_ptr.cpp does not silently fall back to shared_ptr pointer comparison. This was the main thing I wanted to rule out.
• Every Expr subclass implements the now-pure clone()/adjoint()/type_id()/static_equal().
• The NormalOperator::labels() explicit-specialization declarations are correctly placed after the class template and before any use that would trigger implicit instantiation — a genuine ill-formed-NDR fix.

Findings below. The NormalOperatorSequence::static_equal self-comparison I hit while reviewing this is pre-existing rather than yours, so it is split out into #590 against master; worth noting here only because adding clone() at op.hpp:1071 is what first lets those objects into Sum/Product and hence into simplify/canonicalization.

1. Variable::conjugate() does not reset the memoized hash — variable.cpp:50

void Variable::conjugate() { conjugated_ = !conjugated_; }

memoizing_hash() (line 22-25) folds conjugated_ into the hash and asserts memoized-vs-recomputed consistency:

auto v = ex<Variable>(L"x");
v->hash_value();   // memoizes
v->adjoint();      // -> conjugate(), flips conjugated_, no reset
v->hash_value();   // SEQUANT_ASSERT fires in Debug; stale hash in Release

Variable::set_label (line 45-47) and the sibling Power::conjugate() (power.cpp:44) both reset, so this is inconsistent within the same layer. Moved code rather than new breakage, but this PR is the natural place to fix it.

2. Constant::operator*=, +=, -= do not reset the memoized hash — constant.cpp:24, :33, :42

All three mutate value_ and return, while Constant::memoizing_hash() (line 55) asserts the memoized hash still matches value_. Constant::adjoint() (line 19) does reset, so the file is self-inconsistent.

Reachable through Sum::append, which folds constants in place:

Sum s;
s.append(ex<Constant>(1));
s.hash_value();          // Sum::memoizing_hash -> hash::range over dereferenced
                         // summands, memoizing the Constant summand's own hash
s.append(ex<Constant>(2));  // sum.hpp:135 -> summands_[i].as<Constant>() += *summand
                            // mutates the resident Constant 1 -> 3 in place
s.hash_value();          // assert fires in Debug; stale hash silently used in Release

Note Sum::append resets the Sum's hash, not the folded-into Constant's.

3. Expr::not_implemented() is declared but no longer defined — expr.hpp:485

Exception not_implemented(const char *fn) const;

The definition was deleted from expr.cpp along with the throwing default operators, and git grep finds no other definition in the tree. It links today only because nothing calls it. Any future use — in-class, or from a derived class in another TU, since the name is still visible in the inherited context — becomes an undefined reference at link time rather than a compile error. Suggest deleting the declaration.

4. clone() and adjoint() docs still describe the removed throwing default — expr.hpp:82, :244

/// @note - must be overridden in the derived class.
///       - the default implementation throws an exception
virtual ExprPtr clone() const = 0;
...
/// @note base implementation throws, must be reimplemented in the derived class
virtual void adjoint() = 0;

Both are = 0 now. Since the point of the PR is that a missing override is a compile error rather than a runtime throw, these comments currently assert the opposite of the new contract.

5. Product::operator*= is still virtual with nothing to override — product.hpp:364

The Expr base declaration was removed in this PR and nothing derives-and-overrides it. Sum::operator+=/-=, Constant::operator*=/+=/-= and Power::operator*= all correctly dropped virtual in the same change, so this reads as an oversight; it leaves a vestigial vtable slot that suggests base-level dispatch which no longer exists.

Review

WARNING this is Claude-generated, reviewed by me. Unlike my last pass, this one comes with fixes: I pushed four commits to this branch (92c8eeb2..25ada930) rather than leaving a list of nits. Revert anything you disagree with.

Reviewed the full diff (20 files, +1466/−1285) against merge-base 3168a655, plus surrounding context in expr.hpp, expr_ptr.hpp, op.hpp, wick.impl.hpp and tensor_network/v1.cpp. Line references are as of 92c8eeb2.

My 2026-08-13 review is fully addressed

All five items landed in the 2026-08-20 force-push — Variable::conjugate() and the three Constant in-place operators now reset the memoized hash, Expr::not_implemented() is gone, the stale clone()/adjoint() @notes are gone, and Product::operator*= is non-virtual. Thanks.

One bookkeeping note: the resolved product.hpp thread with @ajay-mk concluded to keep virtual on Product::operator*=, and the code now does the opposite. Removing it is right — neither CProduct nor NCProduct overrides operator*=, and both keep Product's type_id(), so prefactor.as<Product>() *= *factor still reaches the same body — but the thread and the code disagree, so it's worth a line here lest it get "fixed" back.

Verification of the mechanical rewrites

All eight *expr op= *that → expr.as<T>() op= *that sites, and each is type-safe:

• expr_ptr.cpp:52,68,84 — inside is<Sum>() / is<Product>() guards.
• sum.cpp:28,54,79 — guarded by summand->is<Constant>() plus the constant_summand_idx_ invariant.
• v1.cpp:461 — canon_byproduct is ex<Constant>(1) at v1.cpp:62, same function (canonicalize, lines 59–470), never rebound; re-asserted at v1.cpp:467.
• wick.impl.hpp:834 — as above.

Sum::canonicalize_impl's this->swap(*new_sum) → using std::swap; swap(*this, *new_sum) is equivalent: there is no free sequant::swap, the deleted member was never reachable from that unqualified call anyway (the block-scope using std::swap shadows class scope), and std::swap performs exactly the move-ctor/two-move-assign sequence the member did.

Every Expr subclass in the tree implements the four now-pure virtuals — Tensor, Operator<S>, NormalOperator<S>, NormalOperatorSequence<S> (clone() added here at op.hpp:1071), mbpt::Operator<void,S>, and the three test fixtures.

Fixed: CProduct/NCProduct were sliced by clone()

This is the one with correctness weight, and it predates the PR — but it is squarely the thing this PR is about, so I fixed it here (1ad9644).

Product::clone() returns ex<Product>(this->deep_copy()) and neither subclass overrode it. Making Expr::clone() pure virtual does not catch this: the base override exists, so the compiler is satisfied and the derived type is silently dropped. Since sequant::adjoint(const ExprPtr&) is clone() then adjoint(), the slice was observable:

• adjoint() of a CProduct reversed its factors, which CProduct::adjoint() deliberately does not do;
• adjoint() of an NCProduct yielded a Product, whose is_commutative() is a recursive pairwise commutes_with check instead of an unconditional false — so canonicalization may reorder factors that must not be reordered.

Reachable from Sum::adjoint() over NCProduct summands, among others. Both clone() overrides are added, with regression tests in test_expr.cpp's clone section. I confirmed both tests fail on 92c8eeb2 and pass after:

test_expr.cpp:518: FAILED:
  REQUIRE( e_adj.as<Product>().factors()[0]->as<Adjointable>().v == -1 )
with expansion: 2 == -1          # CProduct's factors got reversed

test_expr.cpp:531: FAILED:
  REQUIRE_FALSE( e_clone.as<Product>().is_commutative() )
with expansion: !true            # NCProduct's clone reports itself commutative

Fixed: three smaller things

• NCProduct::adjoint() silently switched range library (53e3585). It moved to std::views::reverse | std::views::transform while Product::adjoint() and CProduct::adjoint() in the same file kept range-v3, and the result is still handed to ranges::begin/ranges::end. Behavior-equivalent, but an unexplained switch inside a commit whose message is "Cleanly separate (N)CProduct impl". Reverted to views::. Also fixed the comment above it, which was copy-pasted from CProduct and claims no reversal is needed — NCProduct::adjoint() does reverse, as it must.
• product.cpp pulled <range/v3/all.hpp> (da3050f). The code it absorbed from expr.cpp was served by nine targeted headers; all.hpp is a well-known compile-time sink and this TU already includes tensor_network.hpp. Restored the targeted list. Also added <SeQuant/core/algorithm.hpp> explicitly — bubble_sort only resolved transitively through tensor_canonicalizer.hpp. Worth knowing that CI builds this with CMAKE_UNITY_BUILD=ON, so a missing include here can be masked by whatever TU it gets concatenated with; I verified product.cpp compiles standalone.
• Leftovers (25ada93): stray ; after the out-of-line Product::type_id() and Sum::type_id() definitions; the dead using std::swap; in Product::adjoint(), which no longer has a swap() call beside it now that Sum::swap() is gone; and Product::is_commutative()'s @note this is memoizing, which the implementation contradicts.

Not changed — two things for you

• CProduct(Product&&) / NCProduct(Product&&) now move instead of copy (product.cpp:337,381): Product(other) became Product(std::move(other)). That is a genuine fix — the old code copied from an rvalue parameter — and it is safe, since the only in-tree caller (operator^ in expr_ptr.cpp) binds an lvalue and still selects the copy ctor. But it is a semantic change buried in a commit that reads as pure motion; worth its own line in the commit message. I left the code alone.
• Expr::to_latex() is still a throwing non-pure default while clone()/adjoint()/type_id()/static_equal() went pure. If that is deliberate — classes with no sensible LaTeX form — fine, but the asymmetry isn't documented, and I didn't want to invent a rationale for it. Your call whether to make it pure or to add a @note.

Verification

Configured a non-unity Debug build (clang++, -Werror -Wall -Wpedantic -Wextra) so every TU compiles standalone. unit_tests-sequant: all 6694 assertions in 62 test cases pass. clang-format-17 --dry-run --Werror clean on all four touched files.

Nothing else blocks merge from my side.